Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(protocol-designer): TC renders for OT-2 and fix isSlotEmpty logic #14177

Merged
merged 3 commits into from
Dec 13, 2023

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Dec 12, 2023

closes RQA-2062

Overview

Fixes 3 bugs:

  1. the TC wasn't rendering at all on the deck for the OT-2
  2. the "getIsSlotEmpty" logic for the OT-2 didn't work completely
  3. filter out slot 12 in the OT-2 new location dropdown

Test Plan

Create an OT-2 protocol and add a thermocycler (gen1 or gen 2). Go to the deck map view and see that it renders correctly and that you can add a labware and the 4 slots are considered full. Move the tiprack into another slot and see that slot 1 and 2 are empty and available to add labware into.

Create a move labware step and see in the new location dropdown that slot 12 is not selectable

Changelog

  • pd is still special-casing GEN1 thermocyclers with the 'span7_8_10_11` slot, so i fixed the logic to change that to slot 7 in the deck setup component. we can probably stop special casing it in the future, post PD 8.0
  • fix logic in isSlotEmpty util by mapping the slots to the cutouts and comparing that. the reason why there was an error only with the ot-2 is because the slot would be 1 or 2 and the trash bin is in cutout12, which includes 1 and 2.
  • add 12 as an addressable area type for the ot-2
  • filter out slot 12 in the selector for the new location dropdown

Review requests

see test plan

Risk assessment

low

@jerader jerader requested a review from a team December 12, 2023 19:16
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Merging #14177 (8fe1d2d) into edge (a32571e) will decrease coverage by 0.02%.
Report is 2 commits behind head on edge.
The diff coverage is 10.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #14177      +/-   ##
==========================================
- Coverage   70.40%   70.38%   -0.02%     
==========================================
  Files        2510     2510              
  Lines       71201    71281      +80     
  Branches     8971     9010      +39     
==========================================
+ Hits        50127    50172      +45     
- Misses      18880    18908      +28     
- Partials     2194     2201       +7     
Flag Coverage Δ
app 67.63% <ø> (ø)
protocol-designer 44.62% <10.00%> (+0.07%) ⬆️
shared-data 75.05% <ø> (ø)
step-generation 86.90% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...igner/src/top-selectors/labware-locations/index.ts 3.57% <ø> (ø)
...otocol-designer/src/components/DeckSetup/index.tsx 0.00% <0.00%> (ø)
protocol-designer/src/step-forms/utils/index.ts 36.92% <14.28%> (-0.79%) ⬇️

... and 2 files with indirect coverage changes

@jerader jerader requested a review from a team as a code owner December 12, 2023 20:45
@jerader jerader removed the request for review from a team December 12, 2023 20:45
@@ -40,6 +40,7 @@ export type OT2AddressableAreaName =
| '9'
| '10'
| '11'
| '12'
Copy link
Collaborator Author

@jerader jerader Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke to Brent about this and he said that its fine that it'll be merged into edge and not the release branch since no place in the app lists all the addressable areas for the ot-2.

@@ -39,6 +39,7 @@ import {
TRASH_BIN_ADAPTER_FIXTURE,
WASTE_CHUTE_CUTOUT,
} from '@opentrons/shared-data'
import { SPAN7_8_10_11_SLOT } from '../../constants'
Copy link
Contributor

@koji koji Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit
Maybe adding _ between SPAN and 7 if the changes wouldn't be a lot.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this const is used all over unfortunately so i'll just keep it as is. I hope we can deprecate the usage completely at some point.

Copy link
Contributor

@koji koji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a couple of comments, but the changes look good to me.

@jerader jerader merged commit 5c952b7 into edge Dec 13, 2023
43 of 44 checks passed
@jerader jerader deleted the pd_fix-tc-and-is-slot-empty branch December 13, 2023 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants